Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements the editor title in RN for Gutenberg. #10794

Merged
merged 14 commits into from
Jan 28, 2019

Conversation

diegoreymendez
Copy link
Contributor

@diegoreymendez diegoreymendez commented Jan 10, 2019

IMPORTANT:

This is a work in progress. This PR is NOT ready for merging.

Description:

Implements the editor title in RN for Gutenberg, also removes the native title.

titleinreactnative

Related PRs:

gutenberg: WordPress/gutenberg#13199
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#450

Testing:

Test 1:

  1. Run the app.
  2. Make sure Gutenberg is enabled.
  3. Open a Gutenberg post and edit the title.
  4. Update the post, close the editor.

Make sure the post title has been udpated.

Test 2:

  1. Run the app.
  2. Make sure Gutenberg is enabled.
  3. Open a Gutenberg post and edit the title.
  4. Close the editor without saving.

Make sure the post title hasn't changed.

Test 3:

  1. Run the app.
  2. Make sure Gutenberg is enabled.
  3. Create a new post.

Make sure the placeholder shows up and the title is empty.

Test 4:

Make sure multiline titles work fine.

Test 5:

Make sure pressing ENTER does not insert a newline. Later on we can change this to move the focus to the content but that's outside the scope of this PR.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 10, 2019

2 Warnings
⚠️ This PR is tagged with ‘DO NOT MERGE’.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@pinarol
Copy link
Contributor

pinarol commented Jan 15, 2019

When we tap the close button it discards the title changes and closes the editor immediately.

title2

@pinarol
Copy link
Contributor

pinarol commented Jan 15, 2019

Another thing that needs to be addressed, we were able to focus on the title field when we create a new post. This behavior also seems to be not working anymore. It's ok if we create a new issue for this to handle later.

title3

Podfile Outdated
@@ -96,7 +96,7 @@ target 'WordPress' do
## React Native
## =====================
##
pod 'Gutenberg', :git => 'http://github.com/wordpress-mobile/gutenberg-mobile/', :commit => '704d003adc1f1b76ee0978aa54e05a36fb16371d'
pod 'Gutenberg', :git => 'http://github.com/wordpress-mobile/gutenberg-mobile/', :commit => 'e98f6fcf21cac614192ef189e9514ec7f4a03af4'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive me if you already know about this, there's a recent agreement about not depending on commit numbers anymore even on develop branch. So I think we'll need to update the commit dependencies to a beta tag before merging our PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, but I'd suggest to publish a tag once the review process is complete to avoid multiple tags per PR.

Copy link
Contributor

@pinarol pinarol Jan 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was already agreeing on that sorry if I wasn't clear

So I think we'll need to update the commit dependencies to a beta tag before merging our PRs

Anyway after chatting on this, this conversation might even be outdated

@pinarol
Copy link
Contributor

pinarol commented Jan 16, 2019

Another thing that needs to be addressed, we were able to focus on the title field when we create a new post. This behavior also seems to be not working anymore. It's ok if we create a new issue for this to handle later.

@etoledom was this sth you did as a workaround for alpha? (I mean focusing to empty title at start) As far as I remember we need to focus to title at start weather it is empty or not for beta, if so, we can handle all focusing to title issues separately under this issue #10594 (comment)

@etoledom
Copy link
Contributor

We followed @iamthomasbishop's design feedback indications and implemented the focusing on keyboard for new posts only.

wordpress-mobile/gutenberg-mobile#357 (comment)

I also agree with @diegoreymendez that it would be good to reimplement this behavior on a different PR. 👍

@pinarol
Copy link
Contributor

pinarol commented Jan 16, 2019

We followed @iamthomasbishop's design feedback indications and implemented the focusing on keyboard for new posts only.

wordpress-mobile/gutenberg-mobile#357 (comment)

I also agree with @diegoreymendez that it would be good to reimplement this behavior on a different PR. 👍

Thanks a lot for clarifying this @etoledom . One small thing, I was quoting myself so I was already agreeing on handling this on a separate PR :)

@diegoreymendez
Copy link
Contributor Author

The only remaining issue is the focus-on-open problem. It's my understanding that this issue will cover it? #10594 (comment)

This is ready for another round.

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works and looks good! 🎉

@iamthomasbishop
Copy link

Looking great @diegoreymendez! Two minor things:

  • I believe the desired title placeholder label is "Add a Title"
  • We can probably get rid of the bottom border on the title field, unless we want to leave that for a separate ticket

Also curious how this looks/works in:

  • Landscape iPhone
  • iPad in landscape

Thanks again, Diego!

@diegoreymendez
Copy link
Contributor Author

diegoreymendez commented Jan 28, 2019

@iamthomasbishop - I'll be spawning an issue for the placeholder correction. There's already an issue for the border luckily.

Those are next in my list of things to-do, but I'll start by merging this PR as-is, to make sure everyone can see the RN title for starters.

@diegoreymendez diegoreymendez merged commit a6518d6 into develop Jan 28, 2019
@diegoreymendez diegoreymendez deleted the gutenberg/post-title-in-rn branch January 28, 2019 16:46
@diegoreymendez diegoreymendez added [Type] Enhancement Gutenberg Editing and display of Gutenberg blocks. and removed [Status] DO NOT MERGE labels Jan 28, 2019
@diegoreymendez
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Editing and display of Gutenberg blocks. [Type] Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants